Packed, typed object ids #229

daboross · 2019-08-19T04:23:01Z

Resolves #51, resolve #192.

This is based on an initial implementation @Dessix made, but I removed some of the optimizations in favor of simpler code, and created a separate untyped version.

This implements the binary/packed 12-byte storage described in #192 for object ids, and implements fairly efficient transfers of these 12-byte values between rust and JS. All of the translations, formatting and parsing are unit tested for reliability (since there's no reliance on a screeps server for that)

This also implements a typed version of the id, ObjectId<T>. The type is purely for type inference and optional safety - it has a into_type() method to change the inner type arbitrarily, and can be freely transformed into and from untyped object ids. My thought was that allowing this kind of "free" transmutation would mean we can have serialization and deserialization which just works, and doesn't have to do anything like calling into JS and checking Game.getObjectById() to ensure it's the right type. It means we need that one technically uneccessary check if you get an ID from a thing and then immediately use get_object_typed during the same tick, but I think the advantages are worth it.

ASalvail

It might be nice to implement a few more Into trait for `ObjectId:

ArrayString
String
T
... and From
[u32;3]
ArrayString
String

It'd make using ObjectId much more convivial.

You might argue this is best left for another PR though.

ASalvail · 2019-08-20T15:15:43Z

src/local/object_id/raw.rs

+    /// ```
+    pub fn from_packed_js_val(packed_val: Reference) -> Result<Self, ConversionError> {
+        let mut packed = [0u32; 3];
+        // TODO: make this more efficient, once we get mutable UnsafeTypedArrays.


Do you mind switching this to a tracking issue? We might never see it again otherwise :P
(Feel free to leave the todo)

Opened #230.

ASalvail · 2019-08-20T15:24:36Z

src/objects.rs

-        js_unwrap!(@{self.as_ref()}.id)
+    /// Retrieves this object's id as an untyped, packed value.
+    ///
+    /// This has no major differences from [`HasId::id`] besides the return


besides -> beside?

Right - I think I almost never use besides correctly...

Edit: looking stuff online seems to come up with conflicting information. I think I'll just switch to "except for" for clarity.

Meh, I'm not married to either, hence the question mark. Feel free to leave it like that.

daboross · 2019-08-20T19:50:22Z

With the other conversions - I'm happy to add Into<ArrayString<[u8; 24]>>, Into<String> and From<[u32; 3]> - those seem natural.

I think From<String> would be a bit of a stretch though - conversion into ObjectId could definitely fail if it isn't in the right format, and we do have a FromStr implementation which allows using string.parse()??

ASalvail · 2019-08-20T19:58:24Z

You're right. Maybe an expensive TryFrom<String>, but that, I'd definitely leave for later.

daboross · 2019-08-20T20:07:00Z

Sounds good!

I forgot you mentioned Into<T> too, and I didn't address that. What would you think of adding that as a method, like try_resolve(&self) -> Result<Option<T>, ConversionError> - maybe with a counterpart resolve(&self) -> Option<T> which panics on incorrect type?

There was a similar try_resolve method in @Dessix's original implementation, but I didn't include it since I was thinking of game::get_object_typed. But it could definitely be put in as a convenience for that function..

Dessix · 2019-08-20T20:19:24Z

I think a combination of try_resolve and resolve sounds good, but I may be biased, as- indeed- I did have that functionality in my own implementation.

There is also potential room for a threadlocal LRU which is per-tick reset caching objects by ID to references, reducing calls out to JS.

ASalvail · 2019-08-20T20:31:28Z

Why not TryInto? I don't get why we don't rely on those here (except for the unsafe stuff) as it seems to be the most natural given the rest of the codebase.

Otherwise, I'm certainly not against resolve and try_resolve.

daboross · 2019-08-20T20:38:33Z

I'd argue against it mostly because it doesn't feel like an into operation - an ID is a fairly separate bit of information from the object it names, and "into" conversions usually mean turning the same bit of data into another form? Like we're getting a lot more than another form of the information contained in ObjectId.

The other reason would be ease of use, since we would be implementing TryInto<Option<T>>. id.resolve() seems like it would be nicer to use (and read) than let x: Option<_> = id.try_into(); or Option::try_from(id). Since the id has type info, a method should be able to work without any annotations, whereas TryFrom / TryInto will always need to rely on type inference.

ASalvail · 2019-08-20T21:00:34Z

Alright, you make a good case, I agree with you!

ASalvail

Thanks for the additions!

daboross added 4 commits August 18, 2019 01:38

Implement RawObjectId, a local packed repr for RoomObject ids

fe8eebf

Run some tests in WASM under node.js

a1d2162

Introduce a typed, packed RoomObject id type

7dee9b1

Use packed ObjectId types in API methods

cdd89da

Dessix approved these changes Aug 20, 2019

View reviewed changes

ASalvail reviewed Aug 20, 2019

View reviewed changes

Clean up wording for HasId::untyped_id

ea3d20d

Add some utility conversions for ObjectId<T> and RawObjectId

ffc11ec

daboross added 2 commits August 20, 2019 13:54

Manually implement many traits for ObjectId to avoid bounds on T

027aebb

Add ObjectId::{resolve, try_resolve} methods

a7f720b

ASalvail approved these changes Aug 20, 2019

View reviewed changes

daboross merged commit 2ff73ea into master Aug 20, 2019

daboross deleted the object-id branch August 20, 2019 21:34

daboross mentioned this pull request Aug 21, 2019

Fix extra fixes / cleanups for object IDs #231

Merged

ASalvail mentioned this pull request Aug 22, 2019

Possibility of typing string values more tightly #29

Open

daboross mentioned this pull request Aug 23, 2019

Add a CHANGELOG entry for ObjectId and RawObjectId #240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packed, typed object ids #229

Packed, typed object ids #229

daboross commented Aug 19, 2019

ASalvail left a comment

ASalvail Aug 20, 2019

daboross Aug 20, 2019

daboross Aug 20, 2019

ASalvail Aug 20, 2019

daboross Aug 20, 2019 •

edited

Loading

ASalvail Aug 20, 2019

daboross commented Aug 20, 2019

ASalvail commented Aug 20, 2019

daboross commented Aug 20, 2019 •

edited

Loading

Dessix commented Aug 20, 2019

ASalvail commented Aug 20, 2019

daboross commented Aug 20, 2019 •

edited

Loading

ASalvail commented Aug 20, 2019

ASalvail left a comment

Packed, typed object ids #229

Packed, typed object ids #229

Conversation

daboross commented Aug 19, 2019

ASalvail left a comment

Choose a reason for hiding this comment

ASalvail Aug 20, 2019

Choose a reason for hiding this comment

daboross Aug 20, 2019

Choose a reason for hiding this comment

daboross Aug 20, 2019

Choose a reason for hiding this comment

ASalvail Aug 20, 2019

Choose a reason for hiding this comment

daboross Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

ASalvail Aug 20, 2019

Choose a reason for hiding this comment

daboross commented Aug 20, 2019

ASalvail commented Aug 20, 2019

daboross commented Aug 20, 2019 • edited Loading

Dessix commented Aug 20, 2019

ASalvail commented Aug 20, 2019

daboross commented Aug 20, 2019 • edited Loading

ASalvail commented Aug 20, 2019

ASalvail left a comment

Choose a reason for hiding this comment

daboross Aug 20, 2019 •

edited

Loading

daboross commented Aug 20, 2019 •

edited

Loading

daboross commented Aug 20, 2019 •

edited

Loading